Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tangle-dapp): Bridge revamp & Router integration #2698

Merged
merged 5 commits into from
Dec 15, 2024

Conversation

devpavan04
Copy link
Member

Summary of changes

  • Bridge revamp & Router integration

Proposed area of change

  • apps/tangle-dapp
  • apps/tangle-cloud
  • libs/tangle-shared-ui
  • libs/webb-ui-components

Copy link

netlify bot commented Dec 14, 2024

Deploy Preview for tangle-dapp ready!

Name Link
🔨 Latest commit a982f35
🔍 Latest deploy log https://app.netlify.com/sites/tangle-dapp/deploys/675eef6cdec8ed00085d059f
😎 Deploy Preview https://deploy-preview-2698--tangle-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 14, 2024

Deploy Preview for tangle-cloud ready!

Name Link
🔨 Latest commit a982f35
🔍 Latest deploy log https://app.netlify.com/sites/tangle-cloud/deploys/675eef6c358ef900081e8c5d
😎 Deploy Preview https://deploy-preview-2698--tangle-cloud.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@yurixander yurixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some suggestions that can be improved

import { BridgeChainsConfigType, BridgeTokenType } from './types';
import { Abi } from 'viem';

const asAbi = (abi: any): Abi => abi as Abi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to avoid using any here? Does it show you an error otherwise?

const routerQuoteParams = {
fromTokenAddress,
toTokenAddress,
amountInWei: amount?.toString() ?? '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen in the case that the amount is an empty string?

Comment on lines 186 to 190
amount: Number(amount?.toString() ?? '0'),
sourceTypedChainId,
destinationTypedChainId,
senderAddress: activeAccount?.address ?? '',
recipientAddress: destinationAddress ?? '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen when the amount is zero or the addresses are empty strings? Should we be handling these edge cases explicitly instead?

}

export type RouterTransferProps = {
routerQuoteData: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there are structured data type for this?

@@ -81,7 +82,9 @@ export type LocalStorageValueOf<T extends LocalStorageKey> =
? LiquidStakingTableData
: T extends LocalStorageKey.ONBOARDING_MODALS_SEEN
? OnboardingPageKey[]
: never;
: T extends LocalStorageKey.EVM_TOKEN_BALANCES
? Record<string, any>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any structured data type for this?

@devpavan04 devpavan04 marked this pull request as draft December 15, 2024 14:40
@devpavan04 devpavan04 marked this pull request as ready for review December 15, 2024 16:03
@devpavan04 devpavan04 merged commit 21c35e6 into develop Dec 15, 2024
15 checks passed
@devpavan04 devpavan04 deleted the PS/vite-bridge branch December 15, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants